Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling the use of looping (for in ..) into Template.php #9401

Merged
merged 12 commits into from
Aug 21, 2017

Conversation

thiagolima-bm
Copy link
Member

@thiagolima-bm thiagolima-bm commented Apr 26, 2017

This update enable the use of loop into template files, allowing the user iterate through collections

Why do I think that this change is important?

This way would be more easy to create dynamic feed/exportation files as: xml, csv and so on.

Manual testing scenarios

$_things = new \Magento\Framework\Data\Collection($this->entityFactory);
$thing_1 = new \Magento\Framework\DataObject();
$thing_1->setName('Richard');
$thing_1->setAge(24);

$thing_2 = new \Magento\Framework\DataObject();
$thing_2->setName('Jane');
$thing_2->setAge(12);

$thing_3 = new \Magento\Framework\DataObject();
$thing_3->setName('Spot');
$thing_3->setLastName('The Dog');
$thing_3->setAge(7);

$_things->addItem($thing_1);
$_things->addItem($thing_2);
$_things->addItem($thing_3);

// you may also use arrays
/*
$_things = [
['name' => 'Richard', 'age' => 24],
['name' => 'Jane', 'age' => 12],
['name' => 'Spot', 'age' => 7],
];
*/

$order = $this->orderRepository->get(1);

$template = new \Magento\Framework\Filter\Template(new \Magento\Framework\Stdlib\StringUtils);

$value = "
    Increment ID: {{var order.increment_id}} <br>
    
    <ul>
        {{loop order.all_visible_items}}
            <li>
                sku: {{var item.sku}}<br>
                name: {{var item.name}}<br>
                price: {{var item.price}}<br>
                quantity: {{var item.ordered_qty}}<br>
            </li>
        {{/loop}}               
    </ul>
    
    
    Customer Email: {{var order.customer_email}}
    
    <ul>
        {{loop things}}
            <li>{{var item.indexCount}} name: {{var item.name}}, lastname: {{var item.lastname}}, age: {{var item.age}}</li>
        {{/loop}}
    </ul>"
;

$template->setVariables(array('order' => $order, 'things' => $_things));

echo $template->filter($value);

Output:

<ul>
    
        <li>
            sku: WS03-XS-Red<br>
            name: Iris Workout Top-XS-Red<br>
            price: 0.0000<br>
            quantity: <br>
        </li>
                   
</ul>


Customer Email: [email protected]

<ul>
    
        <li>1 name: Richard, lastname: , age: 24</li>
        
            <li>2 name: Jane, lastname: , age: 12</li>
        
            <li>3 name: Spot, lastname: , age: 7</li>
    
</ul>

Thiago added 2 commits April 26, 2017 02:36
This update enable the use of loop into template files. Allowing the user iterate through collections
removing unused variables
@bap14
Copy link

bap14 commented Apr 26, 2017

What would the output of this look like? Is it possible to utilize an inline-template call within the loop to customize the display of items?

$delimiter = $construction[2];
$loop_text_to_replace = $construction[3];

if (is_array($objectArrayData) || $objectArrayData instanceof Varien_Data_Collection) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should allow more than arrays and object collections in here.
You should use is_array($data) || ($data instanceof Traversable).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied :)

@adragus-inviqa
Copy link
Contributor

Ah, yes - Varien_Data and Varien_Data_Collection. Good times, good times.

removing m1 variables type and renaming m1 methods
@thiagolima-bm
Copy link
Member Author

thiagolima-bm commented Apr 26, 2017

@bap14 Lets suppose that you have the following template:
`

Increment ID: {{var order.increment_id}}

    <ul>
        {{loop order.all_visible_items}}
            <li>
                sku: {{var item.sku}}<br>
                name: {{var item.name}}<br>
                price: {{var item.price}}<br>
                quantity: {{var item.ordered_qty}}<br>
            </li>
        {{/loop}}               
    </ul>
    
    
    Customer Email: {{var order.customer_email}}
    
    <ul>
        {{loop things.collection}}
            <li>name: {{var item.name}}, lastname: {{var item.lastname}}, age: {{var item.age}}</li>
        {{/loop}}
    </ul>`

The output would be:
` Increment ID: 000000001

    <ul>
        
            <li>
                sku: WS03-XS-Red<br>
                name: Iris Workout Top-XS-Red<br>
                price: 0.0000<br>
                quantity: <br>
            </li>
                       
    </ul>
    
    
    Customer Email: [email protected]
    
    <ul>
        
            <li>name: Richard, lastname: , age: 24</li>
        
            <li>name: Jane, lastname: , age: 12</li>
        
            <li>name: Spot, lastname: , age: 7</li>
        
    </ul>

`

@thiagolima-bm
Copy link
Member Author

@adragus-inviqa I have fixed that. Sorry! M1 is still running in my brain hehe

removing useless delimiter
@orlangur orlangur mentioned this pull request Apr 27, 2017
@orlangur
Copy link
Contributor

Hi guys, I see a lot of thumbs up here which means Community is really in need of such functionality, but...

Do you really think it's a good idea to expand some custom templating language? For example, Twig is a pretty mature technology, has pretty similar syntax and loop out-of-the-box.

There is even some attempt https://github.com/kalenjordan/magento-twigmail to introduce Twig support in email templates for Magento 1 :)

@thiagolima-bm
Copy link
Member Author

@orlangur I would say so but, I do know how much effort will be needed. At least my PR does not affect the actual M2 behaviour. :)

@thiagolima-bm thiagolima-bm changed the title Update Template.php Create loop tag into Template.php Apr 28, 2017
@ishakhsuvarov
Copy link
Contributor

ishakhsuvarov commented May 2, 2017

Hi @thiagolima-bm
I generally like the idea of the changes proposed, however I would encourage you to look into some additional features which may be added to theloop. Current implementation may solve just a single problem which is iteration of the specified collection.
Real life usage may require more versatility to the solution such as access to the information on array keys. Link provided above has some information on that.
If you would like to continue work on this PR we could come up with the minimum list of capabilities for such feature.

Thanks

@thiagolima-bm
Copy link
Member Author

thiagolima-bm commented May 2, 2017

@ishakhsuvarov Hey, I would like to continue. You can come up with the list ASAP

@okorshenko
Copy link
Contributor

Hi @thiagolima-bm do you have any progress on that or should we close this PR?

Enabling use of arrays or collections.
@thiagolima-bm
Copy link
Member Author

@okorshenko Yes. The use of arrays is available too.

@thiagolima-bm
Copy link
Member Author

@ishakhsuvarov Updated.

@orlangur
Copy link
Contributor

orlangur commented Jul 5, 2017

@thiagolima-bm, thanks for considering Twig 👍 I don't think it would require too much efforts but will not insist until I have some proof-of-concept in my hand :)

Sorry for not answering for a long time, I had something to say but somehow lost this thread and then forgot.

What do you think about making your implementation more Twig-like instead of custom syntax?

So, instead of

{{loop order.all_visible_items}}
    <li>
        row number: {{var item.itemCount}}<br>
        sku: {{var item.sku}}<br>
        name: {{var item.name}}<br>
        price: {{var item.price}}<br>
        quantity: {{var item.ordered_qty}}<br>
    </li>
{{/loop}}        

I think it would be better to partially implement capabilities of https://twig.sensiolabs.org/doc/2.x/tags/for.html

{{for item in order.all_visible_items}}
    <li>
        row number: {{var loop.index}}<br>
        sku: {{var item.sku}}<br>
        name: {{var item.name}}<br>
        price: {{var item.price}}<br>
        quantity: {{var item.ordered_qty}}<br>
    </li>
{{/for}}

I think it's enough to implement subset of Twig features consisting of {{for, loop.index, loop.first, loop.last and loop.length. In such case we will make learning curve steeper for many developers and will avoid headache if we ever replace custom email templates with Twig or similar engine.

Also, would be really nice to place sample syntax in filterFor method PHPDoc.

Please share your thoughts.

@thiagolima-bm
Copy link
Member Author

thiagolima-bm commented Jul 7, 2017

@ishakhsuvarov @orlangur

`

Increment ID: {{var order.increment_id}}

    <ul>
        {{for item in order.all_visible_items}}
            <li>
                index: {{var loop.index}}
                sku: {{var item.sku}}
                name: {{var item.name}}
                price: {{var item.price}}
                quantity: {{var item.ordered_qty}}
            </li>
        {{/for}}               
    </ul>
    
    Customer Email: {{var order.customer_email}}
    
    <ul>
        {{for thing in things}}
            <li>{{var loop.index}} name: {{var thing.name}}, lastname: {{var thing.lastname}}, age: {{var thing.age}}</li>
        {{/for}}
    </ul>`

The output would be:
`

Increment ID: 000000001

    <ul>
        
            <li>
                index: 1
                sku: WS03-XS-Red
                name: Iris Workout Top-XS-Red
                price: 0.0000
                quantity: 
            </li>
                       
    </ul>
    
    Customer Email: [email protected]
    
    <ul>
        
            <li>1 name: Richard, lastname: , age: 24</li>
        
            <li>2 name: Jane, lastname: , age: 12</li>
        
            <li>3 name: Spot, lastname: , age: 7</li>
        
    </ul>

`

adding Twig-like syntax
@thiagolima-bm thiagolima-bm changed the title Create loop tag into Template.php Enabling the use of looping (for in ..) into Template.php Jul 9, 2017
@thiagolima-bm
Copy link
Member Author

@ishakhsuvarov @orlangur @okorshenko The changes were made.

@okorshenko okorshenko added this to the July 2017 milestone Jul 19, 2017
@okorshenko okorshenko modified the milestones: July 2017, August 2017 Aug 1, 2017
@thiagolima-bm
Copy link
Member Author

@orlangur I do not know what is happening, but when I run these tests in my machice, everything works fine.

Integration tests works fine.
But, somehow, when running unit tests
Fatal error: Cannot declare class Magento\Framework\Filter\Test\Unit\TemplateTest, because the name is already in use in /home/travis/build/magento/magento2/vendor/magento/framework/Filter/Test/Unit/TemplateTest.php on line 373

@orlangur
Copy link
Contributor

vendor/magento/framework/Filter/Test/Unit/TemplateTest.php you should not commit anything into vendor folder, test is placed in https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Filter/Test/Unit/TemplateTest.php

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 11, 2017

CLA assistant check
All committers have signed the CLA.

@thiagolima-bm
Copy link
Member Author

Thanks, @orlangur! All set.

@okorshenko
Copy link
Contributor

Hi @thiagolima-bm could you please look at failed static tests?

@okorshenko
Copy link
Contributor

Fixed on our side

@thiagolima-bm
Copy link
Member Author

Ok!

@magento-team magento-team merged commit 8fd5123 into magento:develop Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
magento-team pushed a commit that referenced this pull request Aug 21, 2017
[EngCom] Public Pull Requests
 - MAGETWO-71744: Fix issue 10032 #10593
 - MAGETWO-71642: Option to send currency in Google Adwords when using dynamic value #10558
 - MAGETWO-70866: Enabling the use of looping (for in ..) into Template.php #9401
@thiagolima-bm
Copy link
Member Author

@okorshenko somehow, my contributor label is missing :(

@orlangur
Copy link
Contributor

Hi @thiagolima-bm, as we already discussed in scope of another PR, this is not something Magento can handle on its side.

Please make sure all emails used for commits: https://github.com/magento/magento2/pull/9401/commits are associated with your current account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants